ENH: Add gradient unit-sphere normalization function #332
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
+ Coverage 78.87% 79.13% +0.25%
==========================================
Files 28 29 +1
Lines 1884 1912 +28
Branches 203 204 +1
==========================================
+ Hits 1486 1513 +27
- Misses 333 339 +6
+ Partials 65 60 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Any chance that we merge this into #327 and include it in the refactor? |
|
I totally understand and I agree we should keep changes focused. And I also think we want to get the base object changes of #305 merged first, move relevant changes in that PR working on the dwi object into #327 and finally, merge this one into #327's branch. Effectively, the pipeline remains in the same philosophy while reducing developer and reviewer churn. |
Refactor gradient data checks in DWI data class: PR nipreps#325 introduced the row-major convention for gradient data in NiFreeze. However, gradient loading was not tested thoroughly. This resulted in some execution flows that would not guarantee a row-major internal convention or would crash under some circumstances. This commit refactors the gradient data checks and adds thorough testing: - Define all error or warning messages as global variables so that they can be checked exactly in tests. - Ensure that gradients conform to the row-major convention when instantiating the DWI class directly. This allows to separate the gradient reformatting from the dimensionality check with the DWI volume sequence. This simplifies the flow, as the gradient reformatting to the row-major convention does not depend on the number of volumes in the DWI sequence. Also, this makes the flow more consistent with the refactored checks of the NIfTI file-based loading utility function (`from_nii`). - Ensure that the gradients conform to the row-major convention immediately after loading the gradients file in the NIfTI file-based loading utility function (`from_nii`). As opposed to the previous implementation, this allows to load the gradients from a file where data follows either column-major or row-major convention. e.g. In the previous implementation the `if grad.shape[1] < 2:` was making an assumption about the layout and/or one that was wrong because we require 4 columns (direction (x,y,z) + b-value) or rows (if in column-major). The new implementation simplifies the execution flow.
This patch - moves the responsibility of formatting and validating the gradients into the attrs' infrastructure; - removes complexity from ``from_nifti()`` that was completely unnecessary (e.g., motion file can be set after creating the DWI object). - updates tests - skips one test that is unreasonably slow, we need to look into what's going on with ``to_nifti()``.
Improve gradient formatting function robustness by adopting a more defensive approach: - Raise if gradients are `None`. - Raise if gradients are not a numeric homogeneous array-like object. Add the corresponding tests. Take advantage of the commit to test the `validate_gradients` function.
|
Can we change the merge base of this PR to #327 and get this merged there first? I think that would save a fair amount of time |
Add gradient unit-sphere normalization function.
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
b5acdeb to
a05bc94
Compare
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
a05bc94 to
d6b51f9
Compare
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
2 times, most recently
from
776f4df to
d8975c5
Compare
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
d8975c5 to
84d4c95
Compare
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
84d4c95 to
032f5f1
Compare
|
Thanks for having rebased this Oscar; reduces the complexity of what was in place previously. We are getting close. |
Still working on it, give me 20 min |
I'll need some more time because the base branch is a pretty fast moving target... (with history rewrites, which makes the rebase shorter in number of commits but harder in their conflicts) |
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
032f5f1 to
6a81e9a
Compare
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
6a81e9a to
3883853
Compare
Okay, I think it's best if I just make sure this one is in full green and I'll deal with the rebase tomorrow, so we can finally merge. Sorry, I can't pour more time into this today. |
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
9abe7a9 to
3883853
Compare
oesteban
force-pushed
the
enh/normalize-gradients-unit-sphere
branch
from
3883853 to
bb12af7
Compare
2 participants
Add gradient unit-sphere normalization function.